Add getters for RFC 3779 extensions to FFI - #5491
Conversation
|
@reneme if possible I'd like to redeem your offer to assist with something like this :p |
There was a problem hiding this comment.
Pull request overview
This PR extends Botan’s FFI and Python bindings to expose RFC 3779 X.509 certificate extensions (IP Address Blocks and AS Blocks), along with a Python regression test that exercises the new getters.
Changes:
- Add new FFI handle types and getter functions for RFC 3779 IP Address Blocks and AS Blocks extensions.
- Add Python wrapper classes/methods on
X509Certto access these extensions. - Add Python tests validating behavior for missing extensions and expected parsed values.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/ffi/ffi.h | Declares new public C FFI APIs for RFC 3779 extension access. |
| src/lib/ffi/ffi_cert_ext.h | Adds internal FFI struct wrappers for the new extension handle types. |
| src/lib/ffi/ffi_cert_ext.cpp | Implements creation/destruction and getters for IP/AS blocks extensions. |
| src/lib/ffi/info.txt | Registers the new internal header in the FFI module’s header list. |
| src/python/botan3.py | Binds new FFI functions and adds Python-facing extension accessors. |
| src/scripts/test_python.py | Adds Python tests covering new extension getters and error cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
29acf2c to
2b3e572
Compare
|
@reneme could you take a look? |
randombit
left a comment
randombit
left a comment
There was a problem hiding this comment.
Sorry I did not get to the review here in time.
I'm not so confident of the overall shape of this. Having the getter being in term of the extension rather than the certificate is quite unlike the rest of the interface. It would make sense to have the extension as a standalone type when we reach the point of actually being able to create certificates. But even then - there isn't that much value (that I can see) in being able to read the values out of an extension just created, you would just want some setters for that. I think it's better if these were all just functions on botan_x509_cert_t
433d584 to
916ff31
Compare
randombit
left a comment
randombit
left a comment
There was a problem hiding this comment.
Thanks looks good - I think my main concern at this point is the implementation doesn't write to the output parameters in certain error cases, which is fine, but the doc comments should call this out specifically that the values are not written at all if an error occurs. I left notes on some but there are probably others eg min/max
|
I added a disclaimer for each function that in case of an error "<everything that is usually modified> is not modified". For |
randombit
left a comment
randombit
left a comment
There was a problem hiding this comment.
Some more comments. Mostly just quesions on the docs, plus one API change request which I think should be pretty easy to handle.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/lib/ffi/ffi_cert_ext.cpp:162
- Same local-index issue as
..._get_family(): for ipv6==0, anithat actually refers to an IPv6 family can slip past theindex >= addr_blocks.size()check and then yieldsBOTAN_FFI_ERROR_BAD_PARAMETERfrom the type-checking helper, rather than a cleanBOTAN_FFI_ERROR_OUT_OF_RANGE.
const size_t index = ipv6 == 0 ? i : ext->v4_count() + i;
const auto& addr_blocks = ext->addr_blocks();
if(index >= addr_blocks.size()) {
return BOTAN_FFI_ERROR_OUT_OF_RANGE;
}
const auto& addr_choice = addr_blocks.at(index).addr_choice();
| return BOTAN_FFI_ERROR_NO_VALUE; | ||
| } | ||
|
|
||
| const size_t index = ipv6 == 0 ? i : ext->v4_count() + i; |
There was a problem hiding this comment.
I suppose instead of checking for index >= addr_blocks.size() this could check like so:
size_t limit = ipv6 == 0 ? ext->v4_count() : ext->v6_count();
if (i >= limit) {
return BOTAN_FFI_ERROR_OUT_OF_RANGE;
}IMO checking for >= addr_blocks.size() is more easily read as what it actually does, making sure no out of bounds index happens. Checking for the counts directly seems kind of arbitrary, but I don't mind either
There was a problem hiding this comment.
I'd be inclined for checking the index against the v4/v6 count directly (your suggested version is better than copilots), esp as it prevents a (unlikely, definitely application bug) footgun where v4_count + i overflows. The at(index) makes it clear that worst case it's still a checked index.
|
Second commit only for readability, will squash before merge. The CI analysis failure is unrelated I think, ruff seems very unhappy all of a sudden, probably got a version bump |
randombit
left a comment
randombit
left a comment
There was a problem hiding this comment.
LGTM! Just needs a squash and then can merge
Adds some getters for the RFC 3779 extensions to the FFI. The FFI structs are not declared with
BOTAN_FFI_DECLARE_STRUCTto allow them to contain an extram_writablebool. I plan to add methods to build these extensions from scratch once e.g. #4996 is finalized and the FFI can create certificates from scratch, and I want to disallow writing to extension objects that have been obtained from an existing certificate (and can thus not be changed without creating a new certificate). This does prohibit copying the extension from an old cert to a new cert while also adjusting it, but I think this is better solved with a_duplicatemethod instead of allowing writes to all extension objects.